Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Implement Lasso inside Hyperplonk #4

Open
wants to merge 27 commits into
base: main
Choose a base branch
from
Open

Implement Lasso inside Hyperplonk #4

wants to merge 27 commits into from
+2,776 −385

Conversation

DoHoonKim8
Copy link
Owner

@DoHoonKim8 DoHoonKim8 commented Oct 31, 2023 •
edited
Loading

Implement Lasso inside Hyperplonk.
Can run the test

cargo test --release --package plonkish_backend --lib -- backend::lookup::lasso::test::and::test --exact --nocapture
cargo test --release --package plonkish_backend --lib -- backend::lookup::lasso::test::range::test --exact --nocapture

The remaining things to do:

@DoHoonKim8 DoHoonKim8 self-assigned this Oct 31, 2023
@DoHoonKim8 DoHoonKim8 force-pushed the feature/Lasso branch 2 times, most recently from 21ec795 to 1a2c33a Compare October 31, 2023 08:43
@DoHoonKim8 DoHoonKim8 marked this pull request as ready for review November 2, 2023 09:24
@ed255 ed255 self-requested a review November 2, 2023 09:55
@DoHoonKim8 DoHoonKim8 mentioned this pull request Nov 4, 2023
Closed
@DoHoonKim8 DoHoonKim8 marked this pull request as draft November 5, 2023 07:58
@DoHoonKim8 DoHoonKim8 marked this pull request as ready for review November 6, 2023 08:33
ed255
ed255 reviewed Nov 10, 2023
View reviewed changes
Copy link
Collaborator

@ed255 ed255 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initial review.
I've done an overview of the PR, here are some general comments:

  • I didn't review the memory checking, sumcheck and GKR implementations
  • I'm not sure if all the contents of this PR are related to Lasso, or there's extra code (there's a file that seems to implement logup for example)
  • I'm not sure if the code in plonkish_backend/src/frontend/halo2/lookup.rs is used. It defines two traits, but they are unused for now?
  • My understanding is that the codebase starts from Han's plonkish repository and then implements Lasso on top of it. But the git history from the plonkish repository is not here? maybe you copied the code into this repository? I highly recommend implementing Lasso on top of Han's plonkish repository keeping all the commit history from the plonkish repository; because that will make it much easier later to merge this implementation to Han's repo, or backport changes from Han's repo into here.

plonkish_backend/src/backend.rs Show resolved Hide resolved
plonkish_backend/src/backend.rs Outdated Show resolved Hide resolved
plonkish_backend/src/backend/hyperplonk.rs Show resolved Hide resolved
plonkish_backend/src/backend/hyperplonk.rs Show resolved Hide resolved
plonkish_backend/src/backend/hyperplonk/preprocessor.rs Outdated
// let lookup_constraints = lookup_constraints.map(Cow::Borrowed).unwrap_or_else(|| {
// let dummy_challenge = Expression::zero();
// Cow::Owned(self::lookup_constraints(circuit_info, &dummy_challenge, &dummy_challenge).0)
// });
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we include here the degree of the lasso lookup expression used to calculate the lookup indices?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah so this part is building expression to be used for zero sumcheck in HyperPlonK, and previously LogUp sumcheck is batched with HyperPlonK zero sumcheck. In case of Lasso, at the time of implementing it, I thought that Lasso sumcheck cannot be batched into HyperPlonK zero sumcheck since Lasso paper describes invoking sumcheck in the middle of IOP. So I just removed this part and built expression for Lasso lookup independently.

For now, I think we can swap the order of Lasso sumcheck and offline memory checking and then batch sumcheck with HyperPlonK zero sumcheck. Can you share your thought on this? @han0110 @ed255

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that makes sense, ideally we only need to invoke one sum check, but currently the sum check only support batching of same number of variates, so we will need 2 for Lasso + HyperPlonk

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I don't understand this fully.

Imagine we have a circuit with 4 columns a, b, c, d.
Then we have one constraint a + b * c - d (degree = 2)
And one lasso lookup with input expression a * b * c * d (degree = 4)

What should the max_degree of the circuit be?
I have search where max_degree is used and only found it's used to batch permutation checks into chunks. What else does the max_degree affect?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that makes sense, ideally we only need to invoke one sum check, but currently the sum check only support batching of same number of variates, so we will need 2 for Lasso + HyperPlonk

In this case Lasso and HyperPlonk sumchecks have the same number of variates, so I think they can be batched. Am I misunderstanding something?

plonkish_backend/src/backend/hyperplonk/prover.rs Show resolved Hide resolved
plonkish_backend/src/backend/lookup/logup.rs Outdated Show resolved Hide resolved
plonkish_backend/src/frontend/halo2/lookup.rs Outdated Show resolved Hide resolved
@DoHoonKim8
Copy link
Owner Author

  • My understanding is that the codebase starts from Han's plonkish repository and then implements Lasso on top of it. But the git history from the plonkish repository is not here? maybe you copied the code into this repository? I highly recommend implementing Lasso on top of Han's plonkish repository keeping all the commit history from the plonkish repository; because that will make it much easier later to merge this implementation to Han's repo, or backport changes from Han's repo into here.

Thanks for pointing out this. It's my mistake to remove all the git history. I will fix this.

For the reason I transferred the code from plonkish to halo2-lasso because Lasso implementation on top of plonkish makes huge changes to the original codebase which makes impossible to merge into Han's repo later(also merge into here from upstream). So I thought that it would be better to have separate repo only for this work.(Ofc I wrote at readme that this repo is fork of plonkish )

@DoHoonKim8 DoHoonKim8 mentioned this pull request Dec 26, 2023
Open
@zeroqn zeroqn mentioned this pull request Feb 28, 2024
Open
@DoHoonKim8 DoHoonKim8 mentioned this pull request Mar 2, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ed255 ed255 ed255 left review comments

@jeong0982 jeong0982 jeong0982 approved these changes

@han0110 han0110 Awaiting requested review from han0110

Assignees

@DoHoonKim8 DoHoonKim8

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@DoHoonKim8 @han0110 @ed255 @jeong0982